[JENKINS-75986] Refactor `BuildReferenceMapAdapter` by dukhlov · Pull Request #10946 · jenkinsci/jenkins

dukhlov · 2025-08-11T13:03:19Z

AbstractLazyLoadRunMap uses BuildReferenceMapAdapter, but there is a lot of code doing a similar job in both classes.
Also, BuildReferenceMapAdapter implements the Map interface instead of extending AbstractMap, which leads to re-implementing trivial methods such as hashCode(), equals(), toString(), and toArray().

Suggested changes for BuildReferenceMapAdapter:

Reuse the standard collection base classes (AbstractMap, AbstractSet, AbstractCollection) to avoid redundant code.
Treat BuildReferenceMapAdapter strictly as a read-only view (except for the case where iterator remove() delegation is required for core), and remove unnecessary mutation methods.
Move the Spliterator customizations from AbstractLazyLoadRunMap into BuildReferenceMapAdapter so that entrySet(), values(), and keySet() implementations can be reused.
Align BuildReferenceMapAdapter with the weak consistency semantics of RunMap (e.g., size() may be approximate, but isEmpty() should remain reliable).

Testing done

Manual testing on my local environment,

Proposed changelog entries

refactor BuildReferenceMapAdapter to improve code re-usage

Proposed changelog category

/label internal

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jglick

Before the changes are marked as ready-for-merge:

Maintainer checklist

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

…sage - Reuse the standard collection base classes (AbstractMap, AbstractSet, AbstractCollection) to avoid redundant code. - Treat BuildReferenceMapAdapter strictly as a read-only view (except for the case where iterator remove() delegation is required for core), and remove unnecessary mutation methods. - Move the Spliterator customizations from AbstractLazyLoadRunMap into BuildReferenceMapAdapter so that entrySet(), values(), and keySet() implementations can be reused. - Align BuildReferenceMapAdapter with the weak consistency semantics of RunMap (e.g., size() may be approximate, but isEmpty() should remain reliable).

jglick · 2025-08-15T13:59:02Z

core/src/main/java/hudson/model/RunMap.java

    /**
     * Add a <em>new</em> build to the map.
-     * Do not use when loading existing builds (use {@link #put(Integer, Object)}).
+     * Do not use when loading existing builds (use {@link #putAll(Map)}).


Left over from the LazyBuildMixIn change in #10759 I guess.

jglick · 2025-08-15T14:03:22Z

core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java

        entry = iterator.next();
        assertEquals(3, entry.getKey().intValue());
-        assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString(), ".next() precomputes the one after that too");
+        assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString());


As I understand it, this behavioral change is just being lazier about loading builds from the entry set iterator.

dukhlov · 2025-08-15T14:15:47Z

core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java

-                } else
-                    throw new NoSuchElementException();
-                return entryOf(last);
-            }


It how it was before - R next = owner.newestBuild(); - was resolved during iterator initialization,
then next() will resolve the next on at place

jglick · 2025-08-15T14:16:17Z

core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

-        for (R i = start; i != end; ) {
-            i = search(getNumberOf(i) - 1, DESC);
-            assert i != null;
-        }


Unclear to me what this loop was even doing (when assertions are disabled).

This code should be removed in scope of #10759. As far as I understood, this loop was loading builds which belong to subMap interval to Index.

Now AbstractLazyLoadRunMap.core (which replaces Index) has all set of build references available on disk by design

core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter.java

jglick · 2025-08-15T14:34:44Z

core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter.java

+        if (val == null) {
+            return null;
+        }
+        return removeValue(val) ? val : null;


Would be nicer to remove the key without resolving it, but that would violate the remove signature. Not sure if this is ever actually called on an unloaded build to begin with.

I made BuildReferenceAdapter readonly new of core, all modification expected to be done using core directly. But to reuse navigation collection and it's iterator.remove() functionality, I delegated removing to class, resposible of core modification (AbstractLazyLoadRunMap). The logic we have there relies on removeValue(R) method. So looks like there is noting I can do. Maybe in next PRs if I or somebody else find a way

core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter.java

jglick

Minor suggestions, but looks good from what I can tell. I am running some tests in CloudBees CI.

jglick · 2025-08-15T14:45:36Z

(remember to push any follow-up changes as new commits to the PR branch rather than force-pushing, to make it easier for reviewers to track progress)

….java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick · 2025-08-15T18:22:52Z

Compilation errors in test module.

jglick

(once tests here are fixed, and if I can get this tested in CloudBees CI)

basil · 2025-08-15T18:30:42Z

This is a great improvement. I don't think I'll have time to review it in detail, but linking #5942 for some history that might be relevant. Thanks again for the great contribution!

jglick

Testing in CloudBees CI turned up no obviously related failures.

timja

Thanks!

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

A1exKH

LGTM.

dukhlov changed the title ~~test1~~ Refactror BuildReferenceMapAdapter Aug 11, 2025

dukhlov force-pushed the refacrot-lazy-loading-part2 branch 2 times, most recently from b041c39 to c6fbfab Compare August 11, 2025 18:29

comment-ops-bot bot added the internal label Aug 11, 2025

dukhlov force-pushed the refacrot-lazy-loading-part2 branch 10 times, most recently from 45a16a8 to ab0488f Compare August 12, 2025 10:14

dukhlov marked this pull request as ready for review August 12, 2025 13:31

dukhlov changed the title ~~Refactror BuildReferenceMapAdapter~~ [JENKINS-75986] Refactror BuildReferenceMapAdapter Aug 12, 2025

jglick changed the title ~~[JENKINS-75986] Refactror BuildReferenceMapAdapter~~ [JENKINS-75986] Refactor BuildReferenceMapAdapter Aug 12, 2025

dukhlov force-pushed the refacrot-lazy-loading-part2 branch 3 times, most recently from b082ffd to d6f5d21 Compare August 12, 2025 21:40

dukhlov force-pushed the refacrot-lazy-loading-part2 branch from d6f5d21 to 48768b1 Compare August 12, 2025 22:25

timja requested a review from a team August 15, 2025 08:29